From 4730aea7260c633aab32a80f63fca5c717714e09 Mon Sep 17 00:00:00 2001 From: Joshua Baehring Date: Sat, 28 Sep 2024 19:58:37 -0400 Subject: [PATCH] [scudo] Reapply "Update secondary cache time-based release logic" This reapplies commit e5271fef8fd8931370f04702ba2f9e8b2ab0e523. In the event that MTE is turned on, in which case released cache entries may be inserted into the cache, all released cache entries are inserted after `LastUnreleasedEntry`. Additionally, when a cache entry is moved from `Quarantine` to `Entries`, its time field will only be updated if the cache entry's memory has not been released. --- compiler-rt/lib/scudo/standalone/secondary.h | 92 +++++++++++++------- 1 file changed, 59 insertions(+), 33 deletions(-) diff --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h index 2fae29e5a2168..47fdd1471c3a5 100644 --- a/compiler-rt/lib/scudo/standalone/secondary.h +++ b/compiler-rt/lib/scudo/standalone/secondary.h @@ -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 @@ -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 @@ -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(Interval) * 1000000); } } @@ -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(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(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; } - Entries[FreeIndex] = Entry; - Entries[FreeIndex].Next = LRUHead; - Entries[FreeIndex].Prev = CachedBlock::InvalidEntry; - LRUHead = static_cast(FreeIndex); EntriesCount++; // Availability stack should not have available entries when all entries @@ -552,6 +568,9 @@ class MapAllocatorCache { Entries[I].invalidate(); + if (I == LastUnreleasedEntry) + LastUnreleasedEntry = Entries[LastUnreleasedEntry].Prev; + if (I == LRUHead) LRUHead = Entries[I].Next; else @@ -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; @@ -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 class MapAllocator {