Skip to content

Reapply "[scudo] Update secondary cache time-based release logic" #110391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Reapply "[scudo] Update secondary cache time-based release logic" #110391

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 59 additions & 33 deletions compiler-rt/lib/scudo/standalone/secondary.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ class MapAllocatorCache {
// The cache is initially empty
LRUHead = CachedBlock::InvalidEntry;
LRUTail = CachedBlock::InvalidEntry;
LastUnreleasedEntry = CachedBlock::InvalidEntry;

// Available entries will be retrieved starting from the beginning of the
// Entries array
Expand Down Expand Up @@ -321,9 +322,13 @@ class MapAllocatorCache {
}
CachedBlock PrevEntry = Quarantine[QuarantinePos];
Quarantine[QuarantinePos] = Entry;
if (OldestTime == 0)
OldestTime = Entry.Time;
Entry = PrevEntry;
// Update the entry time to reflect the time that the
// quarantined memory is placed in the Entries array
// If an entry has a time value of 0, it has already
// been released, so its time value must remain 0
if (Entry.Time != 0)
Entry.Time = Time;
}

// All excess entries are evicted from the cache
Expand All @@ -334,16 +339,12 @@ class MapAllocatorCache {
}

insert(Entry);

if (OldestTime == 0)
OldestTime = Entry.Time;
} while (0);

for (MemMapT &EvictMemMap : EvictionMemMaps)
unmapCallBack(EvictMemMap);

if (Interval >= 0) {
// TODO: Add ReleaseToOS logic to LRU algorithm
releaseOlderThan(Time - static_cast<u64>(Interval) * 1000000);
}
}
Expand Down Expand Up @@ -523,22 +524,37 @@ class MapAllocatorCache {
// Cache should be populated with valid entries when not empty
DCHECK_NE(AvailableHead, CachedBlock::InvalidEntry);

u32 FreeIndex = AvailableHead;
u16 FreeIndex = AvailableHead;
AvailableHead = Entries[AvailableHead].Next;
Entries[FreeIndex] = Entry;

if (EntriesCount == 0) {
LRUTail = static_cast<u16>(FreeIndex);
// Check list order
if (EntriesCount > 1)
DCHECK_GE(Entries[LRUHead].Time, Entries[Entries[LRUHead].Next].Time);

// Released entry goes after LastUnreleasedEntry rather than at LRUHead
if (Entry.Time == 0 && LastUnreleasedEntry != CachedBlock::InvalidEntry) {
Entries[FreeIndex].Next = Entries[LastUnreleasedEntry].Next;
Entries[FreeIndex].Prev = LastUnreleasedEntry;
Entries[LastUnreleasedEntry].Next = FreeIndex;
if (LRUTail == LastUnreleasedEntry) {
LRUTail = FreeIndex;
} else {
Entries[Entries[FreeIndex].Next].Prev = FreeIndex;
}
} else {
// Check list order
if (EntriesCount > 1)
DCHECK_GE(Entries[LRUHead].Time, Entries[Entries[LRUHead].Next].Time);
Entries[LRUHead].Prev = static_cast<u16>(FreeIndex);
Entries[FreeIndex].Next = LRUHead;
Entries[FreeIndex].Prev = CachedBlock::InvalidEntry;
if (EntriesCount == 0) {
LRUTail = FreeIndex;
} else {
Entries[LRUHead].Prev = FreeIndex;
}
LRUHead = FreeIndex;
if (LastUnreleasedEntry == CachedBlock::InvalidEntry)
LastUnreleasedEntry = FreeIndex;
}
Comment on lines +536 to 556
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here seems a little bit too complicated (at least in terms of readability). Can we simplify it as

if (Entry.Time == 0) {
   if (LastUnreleasedEntry ...) {} else {}
} else {
   if (LastUnreleasedEntry ...) {} else {}
}

In addition, single line statement of if-else doesn't need brackets

Copy link
Contributor Author

@JoshuaMBa JoshuaMBa Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue is that in the case that Entry.Time == 0 and LastUnreleasedEntry != CachedBlock::InvalidEntry, then the code will be exactly the same as if we were just inserting the entry at the head of the LRU list. I was thinking that maybe we can just have a function called insertBefore() that inserts a cache entry before a certain index. Then our code could just be

if (Entry.Time == 0 && LastUnreleasedEntry != CachedBlock::InvalidEntry) {
   // Inserts `Entry` right after `LastUnreleasedEntry`
   insertBefore(Entry, Entries[LastUnreleasedEntry].Next)
} else {
    // Inserts `Entry` before `LRUHead`
    insertBefore(Entry, LRUHead)
}

(where insertBefore(CachedBlock::InvalidEntry) would just insert the entry at the tail of the LRU list). Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm thinking of the similar thing. Instead of inserting before, I may do insert after (to align what Scudo list supports)

// Insert `Cur` next to `Prev`
insert(CacheBlock &Prev, CacheBlock& Cur) { ... }

In the caller (i.e., in store())

if (Entry.Time == 0)
  insert(LastUnreleasedEntry, CurBlock);
else
  insert(LRUHead, CurBlock);

So every insertion needs to provide the position to insert, this will help the future replacement with Scudo list.

If you also agree with this, it's better to have another CL to do the refactor and have this later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me. Since we want to add this refactoring in a separate CL, should I leave the logic as is or change it as suggested before:

if (Entry.Time == 0) {
   if (LastUnreleasedEntry ...) {} else {}
} else {
   if (LastUnreleasedEntry ...) {} else {}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we have a better way to tell the logic, it's fine to keep that logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good. I'll leave it as is right now and refactor in the next CL.


Entries[FreeIndex] = Entry;
Entries[FreeIndex].Next = LRUHead;
Entries[FreeIndex].Prev = CachedBlock::InvalidEntry;
LRUHead = static_cast<u16>(FreeIndex);
EntriesCount++;

// Availability stack should not have available entries when all entries
Expand All @@ -552,6 +568,9 @@ class MapAllocatorCache {

Entries[I].invalidate();

if (I == LastUnreleasedEntry)
LastUnreleasedEntry = Entries[LastUnreleasedEntry].Prev;

if (I == LRUHead)
LRUHead = Entries[I].Next;
else
Expand Down Expand Up @@ -593,35 +612,39 @@ class MapAllocatorCache {
}
}

void releaseIfOlderThan(CachedBlock &Entry, u64 Time) REQUIRES(Mutex) {
if (!Entry.isValid() || !Entry.Time)
return;
if (Entry.Time > Time) {
if (OldestTime == 0 || Entry.Time < OldestTime)
OldestTime = Entry.Time;
return;
}
inline void release(CachedBlock &Entry) {
DCHECK(Entry.Time != 0);
Entry.MemMap.releaseAndZeroPagesToOS(Entry.CommitBase, Entry.CommitSize);
Entry.Time = 0;
}

void releaseOlderThan(u64 Time) EXCLUDES(Mutex) {
ScopedLock L(Mutex);
if (!EntriesCount || OldestTime == 0 || OldestTime > Time)
if (!EntriesCount)
return;
OldestTime = 0;
for (uptr I = 0; I < Config::getQuarantineSize(); I++)
releaseIfOlderThan(Quarantine[I], Time);
for (uptr I = 0; I < Config::getEntriesArraySize(); I++)
releaseIfOlderThan(Entries[I], Time);
}

// TODO: Add conditional to skip iteration over quarantine
// if quarantine is disabled
for (uptr I = 0; I < Config::getQuarantineSize(); I++) {
CachedBlock &ReleaseEntry = Quarantine[I];
if (!ReleaseEntry.isValid() || !ReleaseEntry.Time ||
ReleaseEntry.Time > Time)
continue;
release(ReleaseEntry);
}

// Release oldest entries first by releasing from decommit base
while (LastUnreleasedEntry != CachedBlock::InvalidEntry &&
Entries[LastUnreleasedEntry].Time <= Time) {
release(Entries[LastUnreleasedEntry]);
LastUnreleasedEntry = Entries[LastUnreleasedEntry].Prev;
}
}
HybridMutex Mutex;
u32 EntriesCount GUARDED_BY(Mutex) = 0;
u32 QuarantinePos GUARDED_BY(Mutex) = 0;
atomic_u32 MaxEntriesCount = {};
atomic_uptr MaxEntrySize = {};
u64 OldestTime GUARDED_BY(Mutex) = 0;
atomic_s32 ReleaseToOsIntervalMs = {};
u32 CallsToRetrieve GUARDED_BY(Mutex) = 0;
u32 SuccessfulRetrieves GUARDED_BY(Mutex) = 0;
Expand All @@ -636,6 +659,9 @@ class MapAllocatorCache {
u16 LRUTail GUARDED_BY(Mutex) = 0;
// The AvailableHead is the top of the stack of available entries
u16 AvailableHead GUARDED_BY(Mutex) = 0;
// The LastUnreleasedEntry is the least recently used entry that has not
// been released
u16 LastUnreleasedEntry GUARDED_BY(Mutex) = 0;
};

template <typename Config> class MapAllocator {
Expand Down
Loading