[compiler-rt] [scudo] Fix the logic of MaxAllowedFragmentedPages (PR #107927)

[via llvm-commits]

https://github.com/ChiaHungDuan updated https://github.com/llvm/llvm-project/pull/107927

>From d9b12ee4cff866fe03a9cbd029d7690f565f8873 Mon Sep 17 00:00:00 2001
From: Chia-hung Duan <chiahungduan at google.com>
Date: Mon, 9 Sep 2024 22:17:28 +0000
Subject: [PATCH 1/2] [scudo] Fix the logic of MaxAllowedFragmentedPages

MTE doesn't support MaxReleasedCachePages which may break the assumption
that only the first 4 pages will have memory tagged.
---
 compiler-rt/lib/scudo/standalone/secondary.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h
index 1a232b9b9fb2d7..7d85832d7fe714 100644
--- a/compiler-rt/lib/scudo/standalone/secondary.h
+++ b/compiler-rt/lib/scudo/standalone/secondary.h
@@ -725,7 +725,7 @@ MapAllocator<Config>::tryAllocateFromCache(const Options &Options, uptr Size,
   uptr EntryHeaderPos;
   uptr MaxAllowedFragmentedPages = MaxUnreleasedCachePages;
 
-  if (UNLIKELY(useMemoryTagging<Config>(Options)))
+  if (LIKELY(!useMemoryTagging<Config>(Options)))
     MaxAllowedFragmentedPages += CachedBlock::MaxReleasedCachePages;
 
   Entry = Cache.retrieve(MaxAllowedFragmentedPages, Size, Alignment,

>From 5f6a5b3e057d9a430755dedcc99996fe1eda9ad4 Mon Sep 17 00:00:00 2001
From: Chia-hung Duan <chiahungduan at google.com>
Date: Tue, 10 Sep 2024 21:26:24 +0000
Subject: [PATCH 2/2] Add a DCHECK and some comments

---
 compiler-rt/lib/scudo/standalone/secondary.h | 25 +++++++++++++-------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h
index 7d85832d7fe714..521395b7ab243f 100644
--- a/compiler-rt/lib/scudo/standalone/secondary.h
+++ b/compiler-rt/lib/scudo/standalone/secondary.h
@@ -72,13 +72,16 @@ namespace {
 struct CachedBlock {
   static constexpr u16 CacheIndexMax = UINT16_MAX;
   static constexpr u16 InvalidEntry = CacheIndexMax;
-  //   * MaxReleasedCachePages default is currently 4
-  //        - We arrived at this value after noticing that mapping
-  //        in larger memory regions performs better than releasing
-  //        memory and forcing a cache hit. According to the data,
-  //        it suggests that beyond 4 pages, the release execution time is
-  //        longer than the map execution time. In this way, the default
-  //        is dependent on the platform.
+  // We allow a certain amount of fragmentation and part of the fragmented bytes
+  // will be released by `releaseAndZeroPagesToOS()`. This increases the chance
+  // of cache hit rate and reduce the overhead to the RSS at the same time. See
+  // more details in the `MapAllocatorCache::retrieve()` section.
+  //
+  // We arrived at this default value after noticing that mapping in larger
+  // memory regions performs better than releasing memory and forcing a cache
+  // hit. According to the data, it suggests that beyond 4 pages, the release
+  // execution time is longer than the map execution time. In this way,
+  // the default is dependent on the platform.
   static constexpr uptr MaxReleasedCachePages = 4U;
 
   uptr CommitBase = 0;
@@ -725,8 +728,14 @@ MapAllocator<Config>::tryAllocateFromCache(const Options &Options, uptr Size,
   uptr EntryHeaderPos;
   uptr MaxAllowedFragmentedPages = MaxUnreleasedCachePages;
 
-  if (LIKELY(!useMemoryTagging<Config>(Options)))
+  if (LIKELY(!useMemoryTagging<Config>(Options))) {
     MaxAllowedFragmentedPages += CachedBlock::MaxReleasedCachePages;
+  } else {
+    // TODO: Enable MaxReleasedCachePages may result in pages for an entry being
+    // partially released and it erases the tag of those pages. To support this
+    // feature for MTE, we need to tag those pages again.
+    DCHECK_EQ(CachedBlock::MaxReleasedCachePages, 0U);
+  }
 
   Entry = Cache.retrieve(MaxAllowedFragmentedPages, Size, Alignment,
                          getHeadersSize(), EntryHeaderPos);