[compiler-rt] [scudo] Fix release to OS logic in secondary cache. (PR #103303)

[Joshua Baehring via llvm-commits]

https://github.com/JoshuaMBa updated https://github.com/llvm/llvm-project/pull/103303

>From d1a4ef4b9fcc3e01a84c181fb13660042cad2bb3 Mon Sep 17 00:00:00 2001
From: Joshua Baehring <jmbaehring at google.com>
Date: Tue, 13 Aug 2024 15:54:53 +0000
Subject: [PATCH] [scudo] Add time-based release to OS logic.

In order to preserve the least recently used (LRU)
ordering of the cache in the list of decommitted entries,
the cache entries are released from least recent to most
recent. This also helps to avoid unnecessary scans of the cache
since entries ready to be released (specifically, entries that
are considered old relative to the configurable release interval)
will always be at the tail of the list of committed entries
by the LRU ordering.
---
 compiler-rt/lib/scudo/standalone/secondary.h  | 41 ++++++++-----------
 .../scudo/standalone/tests/secondary_test.cpp | 29 ++++++++++++-
 2 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h
index e3dc8d4ef8247c..956fbb508f2638 100644
--- a/compiler-rt/lib/scudo/standalone/secondary.h
+++ b/compiler-rt/lib/scudo/standalone/secondary.h
@@ -321,8 +321,6 @@ class MapAllocatorCache {
         }
         CachedBlock PrevEntry = Quarantine[QuarantinePos];
         Quarantine[QuarantinePos] = Entry;
-        if (OldestTime == 0)
-          OldestTime = Entry.Time;
         Entry = PrevEntry;
       }
 
@@ -344,8 +342,6 @@ class MapAllocatorCache {
 
       insert(Entry, (Entry.Time == 0) ? DECOMMITTED : COMMITTED);
 
-      if (OldestTime == 0)
-        OldestTime = Entry.Time;
     } while (0); // ScopedLock L(Mutex);
 
     for (MemMapT &EvictMemMap : EvictionMemMaps)
@@ -585,32 +581,28 @@ 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) {
     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)
-      return;
-    OldestTime = 0;
-    for (uptr I = 0; I < Config::getQuarantineSize(); I++)
-      releaseIfOlderThan(Quarantine[I], Time);
-    for (u16 I = EntryLists[COMMITTED].Head; I != CachedBlock::InvalidEntry;
-         I = Entries[I].Next) {
-      if (Entries[I].Time && Entries[I].Time <= Time) {
-        unlink(I, COMMITTED);
-        pushFront(I, DECOMMITTED);
-      }
-      releaseIfOlderThan(Entries[I], Time);
+    for (uptr I = 0; I < Config::getQuarantineSize(); I++) {
+      CachedBlock &Entry = Quarantine[I];
+      if (!Entry.isValid() || Entry.Time > Time)
+        continue;
+      release(Entry);
+    }
+
+    // Release oldest entries first by releasing from tail
+    u16 ReleaseIndex = EntryLists[COMMITTED].Tail;
+    while (ReleaseIndex != CachedBlock::InvalidEntry &&
+           Entries[ReleaseIndex].Time <= Time) {
+      release(Entries[ReleaseIndex]);
+      unlink(ReleaseIndex, COMMITTED);
+      pushFront(ReleaseIndex, DECOMMITTED);
+      ReleaseIndex = Entries[ReleaseIndex].Prev;
     }
   }
 
@@ -619,7 +611,6 @@ class MapAllocatorCache {
   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;
diff --git a/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp b/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp
index e85b6abdb36d22..04b2141cb4212a 100644
--- a/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp
@@ -308,14 +308,14 @@ struct MapAllocatorCacheTest : public Test {
                                    scudo::uptr NumEntries, scudo::uptr Size) {
     for (scudo::uptr I = 0; I < NumEntries; I++) {
       MemMaps.emplace_back(allocate(Size));
-      auto &MemMap = MemMaps[I];
+      auto &MemMap = MemMaps.back();
       Cache->store(Options, MemMap.getBase(), MemMap.getCapacity(),
                    MemMap.getBase(), MemMap);
     }
   }
 };
 
-TEST_F(MapAllocatorCacheTest, CacheOrder) {
+TEST_F(MapAllocatorCacheTest, CacheOrderNoRelease) {
   std::vector<scudo::MemMapT> MemMaps;
   Cache->setOption(scudo::Option::MaxCacheEntriesCount,
                    CacheConfig::getEntriesArraySize());
@@ -336,6 +336,31 @@ TEST_F(MapAllocatorCacheTest, CacheOrder) {
     MemMap.unmap();
 }
 
+TEST_F(MapAllocatorCacheTest, CacheOrderWithRelease) {
+  std::vector<scudo::MemMapT> MemMaps;
+  Cache->setOption(scudo::Option::MaxCacheEntriesCount,
+                   CacheConfig::getEntriesArraySize());
+
+  fillCacheWithSameSizeBlocks(MemMaps, CacheConfig::getEntriesArraySize() - 1,
+                              TestAllocSize);
+
+  // Enable releases and perform a deallocation to trigger releases
+  Cache->setOption(scudo::Option::ReleaseInterval, 0);
+  fillCacheWithSameSizeBlocks(MemMaps, 1, TestAllocSize);
+
+  // Retrieval order should be the inverse of insertion order
+  for (scudo::uptr I = CacheConfig::getEntriesArraySize(); I > 0; I--) {
+    scudo::uptr EntryHeaderPos;
+    scudo::CachedBlock Entry =
+        Cache->retrieve(TestAllocSize, PageSize, 0, EntryHeaderPos);
+    EXPECT_EQ(Entry.MemMap.getBase(), MemMaps[I - 1].getBase());
+  }
+
+  // Clean up MemMaps
+  for (auto &MemMap : MemMaps)
+    MemMap.unmap();
+}
+
 TEST_F(MapAllocatorCacheTest, MemoryLeakTest) {
   std::vector<scudo::MemMapT> MemMaps;
   // Fill the cache above MaxEntriesCount to force an eviction