Skip to content
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.

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

[scudo] Update secondary cache time-based release logic #107507

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

JoshuaMBa
Copy link
Contributor

@JoshuaMBa JoshuaMBa commented Sep 6, 2024

Secondary cache entries are now released to the OS from least recent to most recent entries. This 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. For this same reason, the OldestTime variable is no longer needed to indicate when releases are necessary so it has been removed.

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 6, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Joshua Baehring (JoshuaMBa)

Changes

Secondary cache entries are now released to the OS from least recent to most recent entries. This 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. For this same reason, the OldestTime variable is no longer needed to indicate when releases are necessary so it has been removed.


Full diff: https://github.com/llvm/llvm-project/pull/107507.diff

1 Files Affected:

  • (modified) compiler-rt/lib/scudo/standalone/secondary.h (+31-21)
diff --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h
index 1a232b9b9fb2d7..de3038e66d1b52 100644
--- a/compiler-rt/lib/scudo/standalone/secondary.h
+++ b/compiler-rt/lib/scudo/standalone/secondary.h
@@ -318,8 +318,6 @@ class MapAllocatorCache {
         }
         CachedBlock PrevEntry = Quarantine[QuarantinePos];
         Quarantine[QuarantinePos] = Entry;
-        if (OldestTime == 0)
-          OldestTime = Entry.Time;
         Entry = PrevEntry;
       }
 
@@ -331,9 +329,6 @@ class MapAllocatorCache {
       }
 
       insert(Entry);
-
-      if (OldestTime == 0)
-        OldestTime = Entry.Time;
     } while (0);
 
     for (MemMapT &EvictMemMap : EvictionMemMaps)
@@ -514,6 +509,11 @@ class MapAllocatorCache {
     return (EntriesCount >= atomic_load_relaxed(&MaxEntriesCount));
   }
 
+  inline void advanceDecommitBase() REQUIRES(Mutex) {
+    if (DecommitBase != CachedBlock::InvalidEntry)
+      DecommitBase = Entries[DecommitBase].Prev;
+  }
+
   void insert(const CachedBlock &Entry) REQUIRES(Mutex) {
     DCHECK_LT(EntriesCount, atomic_load_relaxed(&MaxEntriesCount));
 
@@ -532,6 +532,9 @@ class MapAllocatorCache {
       Entries[LRUHead].Prev = static_cast<u16>(FreeIndex);
     }
 
+    if (DecommitBase == CachedBlock::InvalidEntry)
+      DecommitBase = static_cast<u16>(FreeIndex);
+
     Entries[FreeIndex] = Entry;
     Entries[FreeIndex].Next = LRUHead;
     Entries[FreeIndex].Prev = CachedBlock::InvalidEntry;
@@ -549,6 +552,9 @@ class MapAllocatorCache {
 
     Entries[I].invalidate();
 
+    if (I == DecommitBase)
+      advanceDecommitBase();
+
     if (I == LRUHead)
       LRUHead = Entries[I].Next;
     else
@@ -590,35 +596,36 @@ 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);
-  }
 
+    for (uptr I = 0; I < Config::getQuarantineSize(); I++) {
+      CachedBlock &ReleaseEntry = Quarantine[I];
+      if (!ReleaseEntry.isValid() || ReleaseEntry.Time > Time)
+        continue;
+      release(ReleaseEntry);
+    }
+
+    // Release oldest entries first by releasing from decommit base
+    while (DecommitBase != CachedBlock::InvalidEntry &&
+           Entries[DecommitBase].Time <= Time) {
+      release(Entries[DecommitBase]);
+      advanceDecommitBase();
+    }
+  }
   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;
@@ -633,6 +640,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 DecommitBase is the least recently used entry that has not
+  // been released
+  u16 DecommitBase GUARDED_BY(Mutex) = 0;
 };
 
 template <typename Config> class MapAllocator {

@JoshuaMBa JoshuaMBa force-pushed the update_release_to_os_logic branch 2 times, most recently from f404b10 to 647eb80 Compare September 9, 2024 20:53
@JoshuaMBa JoshuaMBa force-pushed the update_release_to_os_logic branch 2 times, most recently from 678048a to add8c65 Compare September 12, 2024 20:10
Secondary cache entries are now released to the OS from least recent
to most recent entries. This 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. For this same reason, the OldestTime variable is no longer
needed to indicate when releases are necessary so it has been removed.
@JoshuaMBa JoshuaMBa changed the title [scudo] Update secondary cache time-based release logic. [scudo] Update secondary cache time-based release logic Sep 12, 2024
@ChiaHungDuan ChiaHungDuan merged commit e5271fe into llvm:main Sep 16, 2024
7 checks passed
@JoshuaMBa JoshuaMBa deleted the update_release_to_os_logic branch September 16, 2024 20:26
Copy link
Contributor

@ChiaHungDuan ChiaHungDuan left a comment

Choose a reason for hiding this comment

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

I haven't got time to reproduce the problem detected by the buildbot. I'll try it later this week and update accordingly.

compiler-rt/lib/scudo/standalone/secondary.h Show resolved Hide resolved
compiler-rt/lib/scudo/standalone/secondary.h Show resolved Hide resolved
@JoshuaMBa
Copy link
Contributor Author

I haven't got time to reproduce the problem detected by the buildbot. I'll try it later this week and update accordingly.

Ok, sounds good. It looks like a scudo unit test failed, which is weird because I'm not getting it on my end. Could be dependent on the configuration, but I also had to do some weird workarounds to get the tests running on my local machine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants